Remove `i128` and `u128` from `improper_ctypes_definitions` #137306

tgross35 · 2025-02-20T06:27:01Z

Rust's 128-bit integers have historically been incompatible with C 1. However, there have been a number of changes in Rust and LLVM that mean this is no longer the case:

Incorrect alignment of i128 on x86 1: adjusting Rust's alignment proposed at Set alignment of i128 to 128 bits for x86 compiler-team#683, implemented at LLVM 18 x86 data layout update #116672.
LLVM version of the above: resolved in LLVM, including ABI fix. Present in LLVM18 (our minimum supported version).
Incorrect alignment of i128 on 64-bit PowerPC, SPARC, and MIPS 2: Rust's data layouts adjusted at llvm: Match new LLVM 128-bit integer alignment on sparc #132422, Update mips64 data layout to match LLVM 20 change #132741, rustc_target: ppc64 target string fixes for LLVM 20 #134115.
LLVM version of the above: done in LLVM 20 LLVM data layout for i128 doesn't match Clang's alignment of __int128_t on 64-bit PowerPC, 64-bit SPARC and 64-bit MIPS llvm/llvm-project#102783.
Incorrect return convention of i128 on Windows: adjusted to match GCC and Clang at Windows x86: Change i128 to return via the vector ABI #134290.

At rust-lang/lang-team#255 (comment), the lang team considered it acceptable to remove i128 from improper_ctypes_definitions if the LLVM version is known to be compatible. Time has elapsed since then and we have dropped support for LLVM versions that do not have the x86 fixes, meaning a per-llvm-version lint should no longer be necessary. The PowerPC, SPARC, and MIPS changes only came in LLVM 20 but since Rust's datalayouts have also been updated to match, we will be using the correct alignment regardless of LLVM version.

repr(i128) was added to this lint in #138282, but is also removed here.

Part of the decision is that i128 should match __int128 in C on platforms that provide it, which documentation is updated to indicate. We will not guarantee that i128 matches _BitInt(128) since that can be different from __int128. Some platforms (usually 32-bit) do not provide __int128; if any ABIs are extended in the future to define it, we will need to make sure that our ABI matches.

Closes: #134288

rustbot · 2025-02-20T06:27:09Z

r? @petrochenkov

rustbot has assigned @petrochenkov.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

tgross35 · 2025-02-20T06:29:50Z

I am not aware of any other incompatibilities at this time, @beetrees is there anything outstanding?

I believe this needs a lang FCP @rust-lang/lang.

@rustbot label +T-lang, +I-lang-nominated, +I-lang-easy-decision

RalfJung · 2025-02-20T07:32:47Z

Cc @workingjubilee

programmerjake · 2025-02-20T08:05:33Z

I may be misunderstanding, but it seems like rustc is removing the fixes from the data layout string when it detects LLVM < 20, so unless that's changed to not remove the 128-bit layout from the layout string, rustc would still be broken on LLVM < 20:

rust/compiler/rustc_codegen_llvm/src/context.rs

Lines 168 to 190 in 6d3c050

    
           if llvm_version < (20, 0, 0) { 
        
               if sess.target.arch == "aarch64" || sess.target.arch.starts_with("arm64") { 
        
                   // LLVM 20 defines three additional address spaces for alternate 
        
                   // pointer kinds used in Windows. 
        
                   // See https://github.com/llvm/llvm-project/pull/111879 
        
                   target_data_layout = 
        
                       target_data_layout.replace("-p270:32:32-p271:32:32-p272:64:64", ""); 
        
               } 
        
               if sess.target.arch.starts_with("sparc") { 
        
                   // LLVM 20 updates the sparc layout to correctly align 128 bit integers to 128 bit. 
        
                   // See https://github.com/llvm/llvm-project/pull/106951 
        
                   target_data_layout = target_data_layout.replace("-i128:128", ""); 
        
               } 
        
               if sess.target.arch.starts_with("mips64") { 
        
                   // LLVM 20 updates the mips64 layout to correctly align 128 bit integers to 128 bit. 
        
                   // See https://github.com/llvm/llvm-project/pull/112084 
        
                   target_data_layout = target_data_layout.replace("-i128:128", ""); 
        
               } 
        
               if sess.target.arch.starts_with("powerpc64") { 
        
                   // LLVM 20 updates the powerpc64 layout to correctly align 128 bit integers to 128 bit. 
        
                   // See https://github.com/llvm/llvm-project/pull/118004 
        
                   target_data_layout = target_data_layout.replace("-i128:128", ""); 
        
               }

beetrees · 2025-02-20T08:06:22Z

All the sizes and alignments of i128 match __int128 on 64-bit targets; I haven't checked the ABIs in depth but I'm not aware of any incompatibilities with __int128 (it would probably be good to use something like abi-cafe to check the ABIs). _BitInt(128) currently has a different alignment than __int128 on x86-64: it would be good to document somewhere which one Rust's i128 is compatible with.

While 32-bit platforms don't support __int128, they do support _BitInt(128), so if the lint is going to be removed on all platforms (not just 64-bit platforms) then I'd expect i128 and _BitInt(128) to be ABI compatible on 32-bit platforms. Spot checking a few targets now, I've discovered:

On x86-32, Clang and GCC give _BitInt(128) an alignment of 4 whereas Rust gives i128 an alignment of 16. Additionally, Rust and GCC pass i128/_BitInt(128) directly, whereas Clang passes _BitInt(128) indirectly.
On 32-bit PowerPC, Rust passes i128 directly in registers whereas Clang passes _BitInt(128) indirectly.

Additionally, the Clang documentation states:

Note: the ABI for _BitInt(N) is still in the process of being stabilized, so this type should not yet be used in interfaces that require ABI stability.

programmerjake · 2025-02-20T08:09:44Z

_BitInt(128) currently has a different alignment than __int128 on x86-64: it would be good to document somewhere which one Rust's i128 is compatible with.

ideally x86-64 would fix its broken ABIs so __int128 and _BitInt(128) have the same ABI.

see https://gitlab.com/x86-psABIs/x86-64-ABI/-/issues/11

beetrees · 2025-02-20T08:10:47Z

I may be misunderstanding, but it seems like rustc is removing the fixes from the data layout string when it detects LLVM < 20, so unless that's changed to not remove the 128-bit layout from the layout string, rustc would still be broken on LLVM < 20:

The alignment is only removed when passing the layout to LLVM, so this will only matter if LLVM uses the alignment itself for e.g. low-level function call ABI details. I'm not aware of any cases that this happens for the affected targets, but I haven't looked into it in depth so it's possible there are cases where it matters.

bjorn3 · 2025-02-20T09:33:46Z

I would expect i128 arguments that are passed on the stack to be aligned to 16 bytes with the fix and to be aligned to 4 or 8 bytes without the fix. This changes the offset where the argument is passed and thus affects the ABI.

nikic · 2025-02-20T09:41:31Z

While 32-bit platforms don't support __int128, they do support _BitInt(128), so if the lint is going to be removed on all platforms (not just 64-bit platforms) then I'd expect i128 and _BitInt(128) to be ABI compatible on 32-bit platforms. Spot checking a few targets now, I've discovered:

Given the fact that i128 maps to __int128, but not _BitInt(128) on x86_64, I don't think that mapping it to _BitInt(128) on other platforms is a good idea.

I think it would be preferable to treat i128 as not a proper FFI type on platforms that do not actually specify an ABI for __int128.

petrochenkov · 2025-02-20T11:39:27Z

r? @workingjubilee

rustbot · 2025-02-20T11:39:30Z

Could not assign reviewer from: workingjubilee.
User(s) workingjubilee are either the PR author, already assigned, or on vacation. Please use r? to specify someone else to assign.

petrochenkov · 2025-02-20T11:39:57Z

r? @RalfJung

RalfJung · 2025-02-20T11:45:39Z

Sorry, I don't have the low level ABI / LLVM knowledge to review this. r? @nikic

workingjubilee · 2025-02-20T14:11:37Z

I don't actually think this will be an easy decision.

beetrees · 2025-02-20T15:42:06Z

I would expect i128 arguments that are passed on the stack to be aligned to 16 bytes with the fix and to be aligned to 4 or 8 bytes without the fix. This changes the offset where the argument is passed and thus affects the ABI.

I've just checked; 64-bit PowerPC, 64-bit MIPS and 64-bit SPARC all appear to only align i128 to 8 bytes when it is passed as an argument on the stack, even with LLVM 20 (this is consistent with the PowerPC64 ELFv2 ABI specification).

workingjubilee · 2025-02-21T09:00:56Z

This FIXME does not seem to be addressed for 64-bit Power:

rust/compiler/rustc_target/src/callconv/powerpc64.rs

Lines 1 to 3 in a18bd8a

    
           // FIXME: 
        
           // Alignment of 128 bit types is not currently handled, this will 
        
           // need to be fixed when PowerPC vector support is added.

bors · 2025-04-20T05:58:01Z

☔ The latest upstream changes (presumably #140043) made this pull request unmergeable. Please resolve the merge conflicts.

…li-obk Add `#[repr(u128)]`/`#[repr(i128)]` enums to `improper_ctypes_definitions` This makes them warn whenever a plain `u128`/`i128` would. If the lang team decides to merge rust-lang#137306 then this can be reverted. Tracking issue: rust-lang#56071

Rollup merge of rust-lang#138282 - beetrees:repr128-not-ffi-safe, r=oli-obk Add `#[repr(u128)]`/`#[repr(i128)]` enums to `improper_ctypes_definitions` This makes them warn whenever a plain `u128`/`i128` would. If the lang team decides to merge rust-lang#137306 then this can be reverted. Tracking issue: rust-lang#56071

workingjubilee · 2025-05-01T06:11:50Z

Is there precedent for improper_ctypes being target-dependent? That seems problematic as the crate author seems likely to not see the diagnostic and users will not see it either when it occurs in a dependency.

AFAIK no: the closest would be AIX having a lint for when Rust's repr(C) struct layout doesn't match the C compiler's, but that's done via a separate uses_power_alignment lint (added in #135552).

note for posterity: this lint was found to be a pile of confusion that wasn't really addressing the right problem. I do not regret merging the lint because meandering down the confused path, despite proving slightly aimless, eventually surfaced the real problems. however, it should probably be considered an example to avoid following.

I do not think this fact should necessarily change anyone's decisions here, just making clear that it was using linting Rust source to solve a problem so deep it was instead about the compiler's model of the target. that problem lives in either rustc's backend crates or LLVM, depending on your perspective.

traviscross · 2025-05-08T03:24:10Z

@rfcbot resolve update-per-nikic

We talked about this in the lang call today, with 3/5 present, and were happy to see this go forward. We noted that we should probably be more clear about the fact that while this lint is designed to catch bugs, it's not the definitive reference on what Rust considers to be ABI compatible with C.

rfcbot · 2025-05-08T03:24:16Z

🔔 This is now entering its final comment period, as per the review above. 🔔

RalfJung · 2025-05-08T06:03:35Z

it's not the definitive reference on what Rust considers to be ABI compatible with C.

It is, however, the only reference that exists for this at the moment, to my knowledge...

rfcbot · 2025-05-18T03:40:52Z

The final comment period, with a disposition to merge, as per the review above, is now complete.

As the automated representative of the governance process, I would like to thank the author for their work and everyone else who contributed.

This will be merged soon.

traviscross

FCP is complete. The diff looks OK to me; r=me also when @workingjubilee has reviewed. This also needs a rebase.

@rustbot author

rustbot · 2025-05-18T09:37:28Z

Reminder, once the PR becomes ready for a review, use @rustbot ready.

tgross35 · 2025-05-18T10:12:49Z

To be clear; this PR removes the lint on all targets and does not add a check against a list of known good/problematic targets. Is that what is desired, or is there still interest in the target-specific lint?

I think it would be a good idea to update the i128 docs to say our i128 is meant to be the same as __int128 on platforms where that exists, but not necessarily with _BitInt(128), which seems to be consensus here. If that sounds reasonable, I'll make that change as well.

traviscross · 2025-05-18T11:35:21Z

Having looked back over the minutes from our call, my read and recollection is that we were OK with doing this without the target-specific check, so this should be OK to go forward just with the rebase and with the documentation. The theme of our conversation was that it's not worth linting on this any longer if it's working well enough on the platforms with which people are actually likely to use this. We were OK if the lint had some false negatives.

@tgross35 and I talked this over just now and double checked some other details. One thing we discussed is that if x86-32 later added a __int128 for C, we'd probably want to adjust things such that extern "C" fn f(_: i128) {} would match it, and that might affect anyone who was expecting stability across Rust versions for that signature on that platform, but that seems niche enough that simply documenting it should be OK.

I think it would be a good idea to update the i128 docs to say our i128 is meant to be the same as __int128 on platforms where that exists, but not necessarily with _BitInt(128), which seems to be consensus here. If that sounds reasonable, I'll make that change as well.

Good idea.

Rust's 128-bit integers have historically been incompatible with C [1]. However, there have been a number of changes in Rust and LLVM that mean this is no longer the case: * Incorrect alignment of `i128` on x86 [1]: adjusting Rust's alignment proposed at rust-lang/compiler-team#683, implemented at rust-lang#116672. * LLVM version of the above: resolved in LLVM, including ABI fix. Present in LLVM18 (our minimum supported version). * Incorrect alignment of `i128` on 64-bit PowerPC, SPARC, and MIPS [2]: Rust's data layouts adjusted at rust-lang#132422, rust-lang#132741, rust-lang#134115. * LLVM version of the above: done in LLVM 20 llvm/llvm-project#102783. * Incorrect return convention of `i128` on Windows: adjusted to match GCC and Clang at rust-lang#134290. At [3], the lang team considered it acceptable to remove `i128` from `improper_ctypes_definitions` if the LLVM version is known to be compatible. Time has elapsed since then and we have dropped support for LLVM versions that do not have the x86 fixes, meaning a per-llvm-version lint should no longer be necessary. The PowerPC, SPARC, and MIPS changes only came in LLVM 20 but since Rust's datalayouts have also been updated to match, we will be using the correct alignment regardless of LLVM version. `repr(i128)` was added to this lint in [4], but is also removed here. Part of the decision is that `i128` should match `__int128` in C on platforms that provide it, which documentation is updated to indicate. We will not guarantee that `i128` matches `_BitInt(128)` since that can be different from `__int128`. Some platforms (usually 32-bit) do not provide `__int128`; if any ABIs are extended in the future to define it, we will need to make sure that our ABI matches. Closes: rust-lang#134288 Closes: rust-lang#128950 [1]: rust-lang#54341 [2]: rust-lang#128950 [3]: rust-lang/lang-team#255 (comment) [4]: rust-lang#138282

tgross35 · 2025-05-18T14:56:17Z

Rebased. I also removed the lint against repr(i128) enums that fell into this group and was added in #138282.

Also added a note about ABI compatibility to the primitive docs.

@rustbot ready

rustbot assigned petrochenkov Feb 20, 2025

rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Feb 20, 2025

tgross35 mentioned this pull request Feb 20, 2025

Proposal: Remove i128/u128 from the improper_ctypes lint rust-lang/lang-team#255

Closed

rustbot assigned RalfJung and unassigned petrochenkov Feb 20, 2025

rustbot assigned nikic and unassigned RalfJung Feb 20, 2025

workingjubilee assigned workingjubilee and unassigned nikic Feb 20, 2025

workingjubilee removed the I-lang-easy-decision Issue: The decision needed by the team is conjectured to be easy; this does not imply nomination label Feb 20, 2025

madsmtm mentioned this pull request Apr 4, 2025

FFI support for i128/u128 madsmtm/objc2#597

Open

rfcbot added final-comment-period In the final comment period and will be merged soon unless new substantive objections are raised. and removed proposed-final-comment-period Proposed to merge/close by relevant subteam, see T-<team> label. Will enter FCP once signed off. labels May 8, 2025

traviscross added I-lang-radar Items that are on lang's radar and will need eventual work or consideration. and removed I-lang-nominated Nominated for discussion during a lang team meeting. labels May 8, 2025

rustbot mentioned this pull request May 18, 2025

Tracking issue for release notes of #137306: Remove i128 and u128 from improper_ctypes_definitions #141206

Open

traviscross approved these changes May 18, 2025

View reviewed changes

rustbot added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels May 18, 2025

tgross35 force-pushed the remove-i128-u128-improper-ctypes branch from ce734ba to 8d82555 Compare May 18, 2025 14:52

rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels May 18, 2025

traviscross approved these changes May 18, 2025

View reviewed changes

tgross35 requested a review from workingjubilee May 18, 2025 15:27

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Remove `i128` and `u128` from `improper_ctypes_definitions` #137306

Remove `i128` and `u128` from `improper_ctypes_definitions` #137306

tgross35 commented Feb 20, 2025 •

edited

Loading

rustbot commented Feb 20, 2025

tgross35 commented Feb 20, 2025

RalfJung commented Feb 20, 2025

programmerjake commented Feb 20, 2025

beetrees commented Feb 20, 2025

programmerjake commented Feb 20, 2025 •

edited

Loading

beetrees commented Feb 20, 2025

bjorn3 commented Feb 20, 2025

nikic commented Feb 20, 2025 •

edited

Loading

petrochenkov commented Feb 20, 2025

rustbot commented Feb 20, 2025

petrochenkov commented Feb 20, 2025

RalfJung commented Feb 20, 2025 via email

workingjubilee commented Feb 20, 2025

beetrees commented Feb 20, 2025

workingjubilee commented Feb 21, 2025

bors commented Apr 20, 2025

workingjubilee commented May 1, 2025

traviscross commented May 8, 2025

rfcbot commented May 8, 2025

RalfJung commented May 8, 2025

rfcbot commented May 18, 2025

traviscross left a comment •

edited

Loading

rustbot commented May 18, 2025

tgross35 commented May 18, 2025

traviscross commented May 18, 2025

tgross35 commented May 18, 2025

Remove i128 and u128 from improper_ctypes_definitions #137306

Are you sure you want to change the base?

Remove i128 and u128 from improper_ctypes_definitions #137306

Conversation

tgross35 commented Feb 20, 2025 • edited Loading

rustbot commented Feb 20, 2025

tgross35 commented Feb 20, 2025

RalfJung commented Feb 20, 2025

programmerjake commented Feb 20, 2025

beetrees commented Feb 20, 2025

programmerjake commented Feb 20, 2025 • edited Loading

beetrees commented Feb 20, 2025

bjorn3 commented Feb 20, 2025

nikic commented Feb 20, 2025 • edited Loading

petrochenkov commented Feb 20, 2025

rustbot commented Feb 20, 2025

petrochenkov commented Feb 20, 2025

RalfJung commented Feb 20, 2025 via email

workingjubilee commented Feb 20, 2025

beetrees commented Feb 20, 2025

workingjubilee commented Feb 21, 2025

bors commented Apr 20, 2025

workingjubilee commented May 1, 2025

traviscross commented May 8, 2025

rfcbot commented May 8, 2025

RalfJung commented May 8, 2025

rfcbot commented May 18, 2025

traviscross left a comment • edited Loading

Choose a reason for hiding this comment

rustbot commented May 18, 2025

tgross35 commented May 18, 2025

traviscross commented May 18, 2025

tgross35 commented May 18, 2025

Remove `i128` and `u128` from `improper_ctypes_definitions` #137306

Remove `i128` and `u128` from `improper_ctypes_definitions` #137306

tgross35 commented Feb 20, 2025 •

edited

Loading

programmerjake commented Feb 20, 2025 •

edited

Loading

nikic commented Feb 20, 2025 •

edited

Loading

traviscross left a comment •

edited

Loading